-
Notifications
You must be signed in to change notification settings - Fork 18
Mario Kart 8 implementation #248
base: main
Are you sure you want to change the base?
Conversation
MatthewL246
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I want to say thank you for the PR! I'm sorry I haven't been around to maintain this project very much.
I'm going to trust you that the server works since I don't have my development/testing environment set up.
| ignore = dirty | ||
| [submodule "repos/mario-kart-8"] | ||
| path = repos/mario-kart-8 | ||
| url = https://github.com/Newtendo-Network/nex_mario_kart_8.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a consistency thing here: tabs can be removed, explicit branch and ignore = dirty can be added.
| + self.s3_region = "it" | ||
| + self.bucket_name = os.getenv('PN_MK8_CONFIG_S3_BUCKET', 'amkj') | ||
| + | ||
| + self.redis_uri = "redis://" + os.getenv('PN_MK8_REDIS_URI', '53.53.53.53:1005') # redis://HOST[:PORT][?db=DATABASE[&password=PASSWORD]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Nice way to grab the server configuration from Docker environment variables even though the original server doesn't support it.
I'm not sure that it makes sense to provide default values to getenv, though, when those defaults are basically nonsense (like the invalid IPs and secrets). It might be better to just let them be None and have the server fail fast in case of accidentally-missing variables.
| run_verbose git submodule foreach "git reset --hard" | ||
| run_verbose git submodule foreach "git clean -fd" | ||
| run_verbose git submodule update --init --checkout | ||
| print_info "Resetting all submodules (including nested submodules)..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit nitpicky but I don't feel like adding this to the info text is necessary.
(same for below)
| +redis | ||
| +grpcio-tools | ||
| +minio | ||
| +nintendoclients |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the CI on ARM failed while installing the dependencies here, while x64 succeeded. I'm seeing error: command 'gcc' failed: No such file or directory. My guess is that one of these packages doesn't have a pre-built wheel for ARM, so we might need to add installing GCC and other build dependencies to the Dockerfile for ARM compatibility.
This pull request adds support for the Mario Kart 8 server.
It has been tested with a few friends and seems to be quite stable, however, several tests should be performed to confirm that everything is working.
There is already an open discussion about this in the issue #125
Known issues that should be investigated